nomycnf: simplify db parameters#4088
Merged
sougou merged 5 commits intovitessio:masterfrom Jul 17, 2018
sougou:nomycnf
Merged
Conversation
* Introduce new db connectivity flags. * Mark old flags as deprecated. * If new flags are set, they supersede legacy flags. * Provide defaults to existing flags so one doesn't have to specify them for normal cases. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
DBConfigs API has been changed: * Conn param members have been made private and are not changeable. * New functions like AppWithDB, etc. have been added. They return newly allocated conn params. * New DBName field can now be set by the app, which can then be combined with conn params as needed. * SideCarDBName and DBName are atomic. There's no worry about possible race while accessing them. * 'WithDB' functions return a conn with the db name set. For cases where a db is always needed, like App, the API only provides a WithDB function. Also, vice-versa. * A test has been added to tabletmanager to show that the new flags work. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
* Remove legacy parameters from examples. * Add comments. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Contributor
Author
It's possible one may want to enable SSL for just the repl user because replication can happen on open networks. The per-user use_ssl flag allows you to select which connections use ssl and which don't. DBConfigs has been refactored to use a map instead of hardcoded user configs. This reduces boilerplating. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
jvaidya
reviewed
Jul 17, 2018
Contributor
jvaidya
left a comment
jvaidya
left a comment
There was a problem hiding this comment.
Looks great on the whole. This will really reduce a lot of confusion, especially once the deprecated parameters are deleted. Thanks for tackling this.
I have a couple comments inline.
go/vt/dbconfigs/dbconfigs.go
Outdated
| SidecarDBName string | ||
| // The flags will change the global singleton | ||
| // TODO(sougou): deprecate the legacy flags. | ||
| func registerUserFlags(dbc *userConfig, name string) { |
Contributor
There was a problem hiding this comment.
- Rename to registerPerUserFlags?
- Rename "name" to "username"?
Contributor
Author
There was a problem hiding this comment.
I can call it key or userKey, because the actual username is a field inside the struct.
| // Check for only one. | ||
| break | ||
| } | ||
| dbConfigs.SidecarDBName.Set("_vt") |
Contributor
There was a problem hiding this comment.
Use the command line parameter rather than hard-coding "_vt".
Contributor
Author
There was a problem hiding this comment.
It's the eventual intent.
The problem is that "_vt" is hard-coded all over the place. Before we can do the command-line, we need to refactor those places to use the sidecar name field.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
jvaidya
approved these changes
Jul 17, 2018
Contributor
jvaidya
left a comment
jvaidya
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first step towards vttablet not depending on mycnf. This PR makes the following changes:
use_sslflag has been added that lets you choose which connections must use SSL. This allows you to selectively enable SSL for certain connections. The use case for this is thereplconnection, because replication can go over open networks.db_host, etc. have now been introduced.